[SPARK-34342][SQL] Format DateLiteral and TimestampLiteral toString#31455
[SPARK-34342][SQL] Format DateLiteral and TimestampLiteral toString#31455wangyum wants to merge 3 commits intoapache:masterfrom wangyum:SPARK-34342
Conversation
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/literals.scala
Outdated
Show resolved
Hide resolved
|
Kubernetes integration test starting |
|
Test build #134828 has finished for PR 31455 at commit
|
|
Kubernetes integration test status failure |
|
Kubernetes integration test starting |
|
Kubernetes integration test status failure |
| override def foldable: Boolean = true | ||
| override def nullable: Boolean = value == null | ||
|
|
||
| private val timeZoneId = DateTimeUtils.getZoneId(SQLConf.get.sessionLocalTimeZone) |
There was a problem hiding this comment.
shall we use def? we don't need to serialize it.
|
Test build #134836 has finished for PR 31455 at commit
|
|
|
||
| private val timeZoneId = DateTimeUtils.getZoneId(SQLConf.get.sessionLocalTimeZone) | ||
|
|
||
| override def toString: String = value match { |
There was a problem hiding this comment.
Does it make sense to have universal conversion of any literal to string via applying of the Cast expression to this literal?
There was a problem hiding this comment.
Sorry, I don't understand your question.
There was a problem hiding this comment.
do you mean this should be
override def toString: String = Cast(this, StringType, timeZoneId).eval().toString
There was a problem hiding this comment.
This will change the current behaviour:
Literal.create(Array[Byte](65, 66, 67), BinaryType).toString // before: 0x414243, after: ABC
Literal.create(Map("a" -> 1)).toString // before: map(keys: [a], values: [1]), after: {a -> 1}
| Literal.create(Array("1", "2", "3"), ArrayType(StringType))) | ||
| } | ||
|
|
||
| test("Date/Timestamp toString") { |
There was a problem hiding this comment.
Maybe JIRA prefix in the test title.
|
Kubernetes integration test starting |
|
Kubernetes integration test status success |
|
Test build #134875 has finished for PR 31455 at commit
|
|
LGTM except for the @MaxGekk comment. |
dongjoon-hyun
left a comment
dongjoon-hyun
left a comment
There was a problem hiding this comment.
I'm +1 for this PR AS-IS because this is scoped preciously for Date/Timestamp literal.
For @MaxGekk 's suggestion, let's proceed in a different PR.
|
@MaxGekk . Could you file a new JIRA and make a PR for your suggestion? |
8 participants
What changes were proposed in this pull request?
This pr format DateLiteral and TimestampLiteral toString. For example:
Before this pr:
After this pr:
Why are the changes needed?
Make the plan more readable.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Unit test.